feat: add sidebar scroll position persistence#75
Conversation
- Add `useScroll` hook with debounced scroll event handling - Add `useScrollToElement` hook to save/restore scroll position via localStorage and in-memory NavigationStateContext - Add `NavigationStateProvider` context for session-level state - Update `Sidebar` component to use `useScrollToElement` with a `useLayoutEffect` workaround (SideBar lacks forwardRef support)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryLow Risk Overview The app layout is wrapped in a new The Sidebar wires this up via
Reviewed by Cursor Bugbot for commit 8e57f94. Bugbot is set up for automated code reviews on this repo. Configure here. |
👋 Codeowner Review RequestThe following codeowners have been identified for the changed files: Team reviewers: @nodejs/nodejs-website Please review the changes when you have a chance. Thank you! 🙏 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…gationStateProvider
|
Kindly merge asap, this is something which is actively causing problems for me while reading docs. |
|
I didn't recall correctly @nodejs/nodejs-website but don't we solve that on ui component ? |
|
Hi @AugustinMauroy, Just to clarify, since the website currently triggers a full browser reload on page navigation instead of behaving like a Single Page Application (SPA), all component states are completely wiped out between pages. Because of this full reload, we can't solve it purely within a UI component's lifecycle or state. We actually need to persist the scroll position across sessions using Let me know what you think! |
Signed-off-by: Ubay Dillah <33889018+ubay1@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8e57f94. Configure here.
| @@ -1,13 +1,13 @@ | |||
| import TableOfContents from '@node-core/ui-components/Common/TableOfContents'; | |||
| import Article from '@node-core/ui-components/Containers/Article'; | |||
| import RemoteLoadableBanner from '@node-core/doc-kit/src/generators/web/ui/components/AnnouncementBanner/RemoteLoadableBanner'; | |||
There was a problem hiding this comment.
Announcement banner removed accidentally
Medium Severity
The layout change drops RemoteLoadableBanner and its import while the PR only describes sidebar scroll persistence. Learn pages no longer render the remote announcement banner that previously appeared site-wide.
Reviewed by Cursor Bugbot for commit 8e57f94. Configure here.
| // Clear any pending debounced calls | ||
| if (timeoutRef.current) { | ||
| clearTimeout(timeoutRef.current); | ||
| } |
There was a problem hiding this comment.
Debounced scroll not saved on exit
Medium Severity
When the scroll listener unmounts, cleanup clears the debounce timer without persisting the latest scrollTop. A user who scrolls the sidebar and follows a doc link within the debounce window keeps a stale position in localStorage, so restore on the next page can be wrong.
Reviewed by Cursor Bugbot for commit 8e57f94. Configure here.
| // listener, sidebarRef.current already points to the real <aside> element. | ||
| useLayoutEffect(() => { | ||
| sidebarRef.current = document.querySelector('aside'); | ||
| }, []); |
There was a problem hiding this comment.
Wrong aside targeted globally
Medium Severity
Scroll persistence binds to document.querySelector('aside'), the first aside in the document. The layout renders NavBar (with a mobile navigation toggler) before the doc SideBar, so scroll save/restore may attach to the navbar drawer instead of the learn sidebar.
Reviewed by Cursor Bugbot for commit 8e57f94. Configure here.
3 participants
useScrollhook with debounced scroll event handlinguseScrollToElementhook to save/restore scroll position via localStorage and in-memory NavigationStateContextNavigationStateProvidercontext for session-level stateSidebarcomponent to useuseScrollToElementwith auseLayoutEffectworkaround (SideBar lacks forwardRef support)#74 fix sidebar scroll position persistence